[Data] Fix: Replace bare raise with TypeError in string concatenation#60795
Open
slfan1989 wants to merge 3 commits intoray-project:masterfrom
Open
[Data] Fix: Replace bare raise with TypeError in string concatenation#60795slfan1989 wants to merge 3 commits intoray-project:masterfrom
slfan1989 wants to merge 3 commits intoray-project:masterfrom
Conversation
Replace the bare `raise` statement in `_to_pa_string_input()` with a proper TypeError that includes a descriptive error message. This ensures string concatenation operations fail with clear feedback when given non-string inputs (e.g., numeric columns). Changes: - expression_evaluator.py: Add TypeError with descriptive message - test_arithmetic.py: Add test for invalid input type rejection Signed-off-by: slfan1989 <slfan1989@apache.org>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request is a solid improvement, replacing a bare raise with a descriptive TypeError for invalid string concatenation operations. The code is also simplified by using early returns. I've added one suggestion to make the error message even more informative by including the specific data type of the invalid input, which aligns with the goal stated in the pull request description. The new unit test is a great addition to prevent regressions.
Comment on lines
102
to
105
| raise TypeError( | ||
| "Expected string or string-like pyarrow Array/ChunkedArray for string " | ||
| f"concatenation, got {type(x).__name__}." | ||
| ) |
Contributor
There was a problem hiding this comment.
The error message can be made more specific to better align with the goal stated in the PR description. When x is a pyarrow.Array or pyarrow.ChunkedArray, using x.type instead of type(x).__name__ will provide the underlying data type (e.g., int64), which is more informative for debugging.
Suggested change
| raise TypeError( | |
| "Expected string or string-like pyarrow Array/ChunkedArray for string " | |
| f"concatenation, got {type(x).__name__}." | |
| ) | |
| type_name = x.type if isinstance(x, (pa.Array, pa.ChunkedArray)) else type(x).__name__ | |
| raise TypeError( | |
| "Expected string or string-like pyarrow Array/ChunkedArray for string " | |
| f"concatenation, got {type_name}." | |
| ) |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
python/ray/data/_internal/planner/plan_expression/expression_evaluator.py
Show resolved
Hide resolved
Replace the bare `raise` statement in `_to_pa_string_input()` with a proper TypeError that includes a descriptive error message. This ensures string concatenation operations fail with clear feedback when given non-string inputs (e.g., numeric columns). Changes: - expression_evaluator.py: Add TypeError with descriptive message - test_arithmetic.py: Add test for invalid input type rejection Signed-off-by: slfan1989 <slfan1989@apache.org>
Comment on lines
100
to
109
| if isinstance(x, (pa.Array, pa.ChunkedArray)) and _is_pa_string_like(x): | ||
| return _pa_decode_dict_string_array(x) | ||
| if isinstance(x, (pa.Array, pa.ChunkedArray)): | ||
| actual_type = str(x.type) | ||
| else: | ||
| raise | ||
| return x | ||
| actual_type = type(x).__name__ | ||
| raise TypeError( | ||
| "Expected string or string-like pyarrow Array/ChunkedArray for string " | ||
| f"concatenation, got {actual_type}." | ||
| ) |
Contributor
There was a problem hiding this comment.
Suggested cleanup:
if isinstance(x, (pa.Array, pa.ChunkedArray)) and _is_pa_string_like(x):
return _pa_decode_dict_string_array(x)
actual_type = str(x.type) if isinstance(x, (pa.Array, pa.ChunkedArray)) else type(x).__name__
raise TypeError(
"Expected string or string-like pyarrow Array/ChunkedArray for string "
f"concatenation, got {actual_type}."
)
Contributor
Author
There was a problem hiding this comment.
Thanks for the suggestions! I’ve updated the PR accordingly—could you please take another look?
Replace the bare `raise` statement in `_to_pa_string_input()` with a proper TypeError that includes a descriptive error message. This ensures string concatenation operations fail with clear feedback when given non-string inputs (e.g., numeric columns). Changes: - expression_evaluator.py: Add TypeError with descriptive message - test_arithmetic.py: Add test for invalid input type rejection Signed-off-by: slfan1989 <slfan1989@apache.org>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This PR fixes a bug in
_to_pa_string_input()where attempting to concatenate string columns with non-string columns (e.g., numeric types) would raise a bareRuntimeErrorinstead of a descriptiveTypeError.Changes:
raisestatement with properTypeErrorthat includes a clear error message indicating expected vs actual input typestest_string_concat_invalid_input_typeto verify the fixBefore: Bare
raisecaused crypticRuntimeError: No active exception to reraiseAfter: Clear
TypeError: Expected string or string-like pyarrow Array/ChunkedArray for string concatenation, got int64.Related issues
Additional information
Example of the fixed behavior: